[CoreCLR] Implement GC bridge #10198

simonrozsival · 2025-06-13T15:15:25Z

Replaces #10185
Implements dotnet/runtime#115506
Builds on top of dotnet/runtime#116310 - this PR is expected to fail to build until this runtime PR is merged and flows into main

Description

TBD

/cc @BrzVlad @jonathanpeppers @grendello

src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

grendello · 2025-06-16T07:49:21Z

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

+		lock (instance.RegisteredInstances) {
+			for (int i = 0; (nuint)i < mcr->ComponentCount; i++) {
+				for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) {
+					var context = (HandleContext*) mcr->Components [i].Contexts [j];


Can we be sure .Contexts [j] will always be HandleContext*? I'd feel "safer" if it was an as statement followed by a null check. I think we should try avoiding exceptions in this code, even if they're very unlikely to happen.

You are right that somebody could use the JavaMarshal.CreateReferenceTrackingHandle in their code and pass a pointer to a different structure. In this case, I am not sure what we can do about it. We cannot use as or is with pointer types, also that memory itself is native and not managed.

I wonder if we need to have a Dictionary<IntPtr, GCHandle> where we would track all the native memory poitners we create and which handles they correspond to. This would replace the weird reference to the GCHandle in WeakPeerReference.

We should do what we can. I had to look up MarkCrossReferencesArgs struct to see what it is and indeed we can't verify that the context is actually a HandleContext* in the current shape of the code. The dictionary idea might be a very good one.

I think what we absolutely must do is to verify that mcr->Components isn't null and that mcr->Components[x].Contexts isn't null before using them here.

We can't do anything to check whether the array subscript is valid, so we unfortunately need to trust the caller that they got the data right.

I will explore the idea with the dictionary and see if it is feasible.

I change the implementation of the value manager to keep a dictionary of the context -> handle mappings and only accept contexts which were produced by us.

I don't think there is a good way to implement this on the native side without too much overhead. We currently assume that all the contexts passed to us by the GC are our contexts of the right "type" with the right size. That might not be necessarily the case, although I think it's extremely unlikely that it would become a problem.

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

src/native/clr/host/gc-bridge.cc

src/native/clr/host/bridge-processing.cc

grendello · 2025-06-18T08:27:50Z

src/native/clr/include/host/gc-bridge.hh

+	class GCBridge
+	{
+	public:
+		static void wait_for_bridge_processing () noexcept;


This is a simple enough method, might want to make it inline here.

It is true that it could be inlined here but I prefer having all the places which touch the mutex+semaphore to be next to each other in the .cc file. If you're ok with it, I would keep this part of the code as is.

Copilot

Pull Request Overview

This PR introduces a new GC bridge to support improved garbage collection coordination between the managed and Java runtimes, replacing #10185 and aligning with the related runtime changes. Key changes include the implementation of GC bridge interfaces and processing logic in native code, updates to pinvoke tables and build configuration, and modifications in the ManagedValueManager and various JNI bridging files to use the new GC bridge callbacks.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/native/clr/include/host/gc-bridge.hh	Adds new structures and functions to support native GC bridge callbacks.
src/native/clr/include/host/bridge-processing.hh	Introduces types and functions for processing cross-references.
src/native/clr/host/pinvoke-tables.include	Updates the internal pinvoke table, increasing the expected count.
src/native/clr/host/gc-bridge.cc	Implements the GC bridge’s processing thread and callback logic.
src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs	Transitions peer management to use the new GC bridge initialization.
(Other JNI-related files)	Replaces outdated references to JNIEnvInit.ValueManager with JniEnvironment.Runtime.ValueManager for consistency.

Comments suppressed due to low confidence (1)

src/Mono.Android/Android.Runtime/JNIEnvInit.cs:40

Since all references now use JniEnvironment.Runtime.ValueManager, consider removing this unused field to avoid confusion and improve code maintainability.

) Context: #10198 This PR contains bits from #10198 which don't depend on new APIs in dotnet/runtime. * Turn `ManagedValueManager` into singleton, for safety/correctness, as we don't want to be calling `JavaMarsha.Initialize()` more than once. * Create `SimpleValueManager` for NativeAOT, which is the "simple" implementation that just leaks... Eventually, `ManagedValueManager` will use the new `JavaMarshal` APIs and `SimpleValueManager` will be used temporarily for NativeAOT. `SimpleValueManager` can be removed in the future when `JavaMarshal` is implemented for NativeAOT.

jonathanpeppers · 2025-06-19T16:08:03Z

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

@@ -20,53 +22,69 @@ class ManagedValueManager : JniRuntime.JniValueManager
 {
 	const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

-	Dictionary<int, List<IJavaPeerable>>?   RegisteredInstances = new Dictionary<int, List<IJavaPeerable>>();
+	Dictionary<int, List<ReferenceTrackingHandle>>? RegisteredInstances = new ();


This code was like this before, but:

Does anything set RegisteredInstances to null?

If not, can we make it readonly and non-nullable?

There are several checks in here like:

if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager));

Should those actually be checking if _context is null?

I agree, we can drop the ? + remove all the null checks.

I am not sure if this class can even be disposed. There is no way to deregister from JavaMarshal so it will keep calling the UCO methods. It seems like Dispose on this class should crash the whole app 🤔 Is JniRuntime.Dispose() part of graceful shutdown of the app? or is that ever called? I don't see how else the VM would be disposed.

Dispose() is probably never called on Android, it might be called on certain desktop unit tests:

https://github.com/dotnet/java-interop/blob/5c48d83c922ca30026110e416b9a6570dbcbbd4c/tests/Java.Interop-Tests/Java.Interop/JniRuntimeTest.cs#L23-L38

jonathanpeppers · 2025-06-19T16:12:06Z

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs


-	static  readonly    Type[]  XAConstructorSignature  = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) };
+	static readonly Type[] XAConstructorSignature = new Type[] { typeof (IntPtr), typeof (JniHandleOwnership) };


nit: maybe we could just put this back to make the diff smaller:

Suggested change

static readonly Type[] XAConstructorSignature = new Type[] { typeof (IntPtr), typeof (JniHandleOwnership) };

static readonly Type[] XAConstructorSignature = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) };

Jon Pryor had some editor that padded with spaces. We don't have to use it, but maybe we can keep old code the way it was.

The padding can make diffs easier to read when adding new C# keywords or changing types, example:

-- public JniObjectReference PeerReference { ++ public unsafe JniObjectReference PeerReference {

I've been struggling with VS Code a lot while working on this PR. It really doesn't like extra white space and "egyptian brackets" :D I will try to revisit this next week.

Yes, something keeps auto-formatting in VS Code, I haven't figured it out yet either...

Best I found is to press Ctrl+Z (Cmd+Z) when I notice it.

Implement GC bridge for CoreCLR

5e92071

jonathanpeppers reviewed Jun 13, 2025

View reviewed changes

src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs Outdated Show resolved Hide resolved

src/Mono.Android/Android.Runtime/AndroidRuntime.cs Outdated Show resolved Hide resolved

grendello reviewed Jun 16, 2025

View reviewed changes

simonrozsival added 5 commits June 17, 2025 08:27

Address review

4e7c284

Split GCBridge into two classes

1205f46

Refactor HandleContext and ReferenceTrackingHandle

369dd18

Update GC Bridge API to respect changes to cross_refs lifetime changes

960945d

Remove unnecessary util methods

212c1cc

grendello reviewed Jun 18, 2025

View reviewed changes

Move logging to separate methods to reduce noise

72ba81b

simonrozsival mentioned this pull request Jun 18, 2025

[GC Bridge] Split ManagedValueManager and SimpleValueManager #10222

Merged

simonrozsival added 4 commits June 18, 2025 12:19

Reduce nesting in gc-bridge.cc

c798a7f

Reduce nesting and other code cleanups

333b131

Address feedback

be2ed77

Thanks copilot

f54d580

simonrozsival requested a review from Copilot June 18, 2025 11:28

Copilot AI reviewed Jun 18, 2025

View reviewed changes

simonrozsival added 3 commits June 19, 2025 09:36

Merge branch 'main' into dev/srozsival/gcbridge-clr

4adf4cc

Fix deadlocks

74f843d

Cleanup

058d6b2

jonathanpeppers reviewed Jun 19, 2025

View reviewed changes


		static readonly Type[] XAConstructorSignature = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) };
		static readonly Type[] XAConstructorSignature = new Type[] { typeof (IntPtr), typeof (JniHandleOwnership) };

[CoreCLR] Implement GC bridge #10198

Are you sure you want to change the base?

[CoreCLR] Implement GC bridge #10198

Uh oh!

Conversation

simonrozsival commented Jun 13, 2025

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!